-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Remove the pre-expansion pass #15980
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Lintcheck changes for b21c720
This comment will be updated if you push new changes |
825beb0 to
870725a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be a general regression on all these lints with non_minimal_cfg taking the biggest hit. Anything within a stripped out item will not be linted, including other attributes on said item. At the minimum I would note this in the known problems of non_minimal_cfg.
In practice actually working properly with lint attributes is probably more important. When cfg's are used solely to add things (e.g. only using #[cfg(feature = ...)]) then running clippy in a way that enables everything will work just fine. Using them for mutually exclusive paths (e.g. target based) will be a problem, especially if it's for small things like setting constant values. When the mutually exclusive parts are large chunks of code clippy would need to be run once for each path to lint them properly anyways.
clippy_lints/src/attrs/mod.rs
Outdated
| /// `cfg_attr` is valid in some places that `rustfmt::skip` is not, e.g. outer attributes on | ||
| /// blocks and inner attributes on inline modules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we easily not lint here by linting from all the other check_* functions instead of check_attributes.
The inner attribute could technically be moved to an outer attribute, but just not linting would be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, they were valid in fewer places than I thought. I think I have all the outer ones but may have missed something
I reverted the inner attribute change since a top level #![rustfmt::skip] isn't valid, I had just assumed it was based on the comment in the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing check_arm, check_trait_item, check_impl_item and check_variant.
The attribute also works on fields and foreign items, but there's no corresponding check function.
a17040f to
ac2220f
Compare
ac2220f to
b21c720
Compare
Fixes #13007
deprecated_cfg_attrnow also lints inner attributes since it can catch the crate level ones, the suggestion is nowMaybeIncorrectas there were some existing cases where the suggestion isn't valid and this adds some new onesdeprecated_clippy_cfg_attrusesstripped_cfg_itemsin addition to a regular early pass as it will typically be in acfgthat evaluates to falsenon_minimal_cfgcan't usestripped_cfg_itemshere though since it only stores the part of thecfgthat evaluated to false whereas the lint needs the wider context, this means the lint no longer fires on code that iscfgd out, which may or may not be expectednon_minimal_cfgnow also lintscfg_attrconditions and lints both emptyany()andall(), suggesting to replace them withtrue/falseas long as the MSRV is 1.88 (rust-lang/rust#138632)changelog: [
non_minimal_cfg] now lints emptyany()and conditions in#[cfg_attr]s